matchBinaries improvements #686

tpapagian · 2023-02-06T17:56:18Z

This PR fixes several issues regarding matchBinaries selector.

The first patch fixes some issues related to matchBinaries selector, but keeps the limit of 4 values. The second patch removes the limit of 4 values per matchBinaries selector. More details on the patches.

Additionally, this PR adds support for NotIn operator in matchBinaries selector.

Signed-off-by: Anastasios Papagiannis tasos.papagiannnis@gmail.com

pkg/selectors/selectors.go

mtardy · 2023-02-20T09:36:30Z

We should update the documentation on matchBinaries limitations in this PR when the doc PR merges!

Is it possible to add the NotIn operator along the way :p?

tpapagian · 2023-02-20T10:27:23Z

We should update the documentation on matchBinaries limitations in this PR when the doc PR merges!

Yes, let's see which PR is merged first and I will update accordingly.

Is it possible to add the NotIn operator along the way :p?

Sure, I will try to do that in this PR!

tpapagian · 2023-02-20T10:28:08Z

Moving to draft in order to do further improvement in this PR.

tpapagian · 2023-02-21T09:24:38Z

I have added more stuff to this PR and now it is ready for review.

bpf/process/bpf_execve_event.c

kkourt

Thanks! This looks like a great improvement!

It took me a while to understand what is happening, so I've added some comment suggestions.

The only requested change is that I think we should check for the maximum length of the binary names and return an error if a user specifies a binary name >255.

Also:

As a follow-up we can also clear entries from the (shared) names_map
when we remove kprobes. For now we also increase the size of that map to
256 entries. This means that we can define up to 256 unique binary names
among all matchBinaries selectors.

Let's create an issue for this? Since the same name may be used by multiple kprobes, I think we would need reference counting to know when to remove an entry. Or we can just scan everything. Another limitation is that we only catch binaries that are exec-ed after the policy started, which might be worth adding to the issue as well.

pkg/selectors/kernel.go

bpf/process/bpf_execve_event.c

bpf/process/types/basic.h

pkg/selectors/selectors.go

bpf/process/types/basic.h

tpapagian · 2023-02-23T10:29:20Z

@kkourt thanks for the review. I have addressed all comments.

I have also created #722 with follow-ups.

kkourt

Thank you! 🚀 🚀

tixxdz

I tried to quickly review, but can continue later since this fixes a bug, however it seems a bit rushed and also would have been better the bug fix and the NotIN in separate PR so we can easily merge the former.

If you are sure about it feel free to merge, but I think those comments need to be addressed later

bpf/process/bpf_execve_event.c

pkg/selectors/kernel.go

pkg/selectors/selectors.go

bpf/lib/generic.h

There are some issues regarding matchBinaries. In this patch we still support up to 4 values in matchBinaries. Increasing this will be a followup. For matchBinaries, we use names_map that has binary names to id translations. During exec events we check if the binary name exists in this map and if that is true we keep that id in the execve_map_value struct. Now we write in the matchBinaries selectors the value 1 everywhere. To fix that we introduce a single global variable that get a new unique ID for each binary specified. We cannot use a separate names_map for each kprobe as they should also be shared with the execve kprobe. We keep a single names_map for all kprobes. Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>

Before that we had a limit to 4 values in the matchBinaries selector. This patch uses a map per kprobe (sel_names_map) to remove this limitation. The current limit is 256 values (the size of the map) and should be enough for all cases. As a follow-up we can also clear entries from the (shared) names_map when we remove kprobes. For now we also increase the size of that map to 256 entries. This means that we can define up to 256 unique binary names among all matchBinaries selectors. Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>

For now we only supported In operator in matchBinaries. This patch adds support for the NotIn operator. Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>

After loading a tracepoint program we should update names_map with new enties in a similar way to kprobes. Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>

Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>

Now, if the process binary does not match these that we have in matchBinaries selector, it will also check the parent binary name. This is not the desired behaviour and this patch removed that. Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>

In the case where the binary name is > 255 characters we simply skip the test. In order to support that we have to filter using data events that can be a follow-up. Generally, 255 characters for binary names should be enough in most cases. Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>

Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>

tpapagian · 2023-02-24T10:15:20Z

I tried to quickly review, but can continue later since this fixes a bug, however it seems a bit rushed and also would have been better the bug fix and the NotIN in separate PR so we can easily merge the former.

I feel that it is fine to have different commits in a single PR that refer to a single (matchBinaries) sub-system. Maybe the PR title was misleading as adding NotIn is not a fix but an improvement. So, I changed the title of this PR to better describe that.

mtardy · 2023-02-24T10:46:40Z

🎉 Thanks @tpapagian

Using the following tracing policy: kprobes: - call: "fd_install" syscall: false args: - index: 0 type: int - index: 1 type: "file" selectors: - matchBinaries: - operator: "In" values: - "/usr/bin/cat" matchArgs: - index: 1 operator: "Equal" values: - "/etc/passwd" - matchBinaries: - operator: "In" values: - "/usr/bin/tail" matchArgs: - index: 1 operator: "Equal" values: - "/etc/shadow" we expect to get events if: [ binary == /usr/bin/cat AND arg1 == /etc/passwd ] OR [ binary == /usr/bin/tail AND arg1 == /etc/shadow ] Using the previous tracing policy and running: /usr/bin/cat /etc/passwd && /usr/bin/cat /etc/shadow && /usr/bin/tail /etc/passwd && /usr/bin/tail /etc/shadow We get events for all of these. The issue is that in #686 we use a single map for all matchBinaries selectors. That makes the previous tracing policy to behave as: [ (binary == /usr/bin/cat OR binary == /usr/bin/tail) AND arg1 == /etc/passwd ] OR [ (binary == /usr/bin/cat OR binary == /usr/bin/tail) AND arg1 == /etc/shadow ] This patch fixes that issues. We move from a BPF_MAP_TYPE_HASH to a BPF_MAP_TYPE_HASH_OF_MAPS. We index the outter map based on selector ID and the inner map is exactly the same as the one we used in #686. Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>

Y-dc · 2023-03-20T10:02:36Z

nice fix! I found that matchBinaries does not work as expected in version v0.8.4. I tested on main 640361d62d34f05fa5065fb39b118d3121e3205b, kprobe filtering is expected, but tracepoint filtering is not successful. Have you tested tracepoint? @tpapagian

tpapagian · 2023-03-20T10:12:33Z

nice fix! I found that matchBinaries does not work as expected in version v0.8.4. I tested on 640361d62d34f05fa5065fb39b118d3121e3205b, kprobe filtering is expected, but tracepoint filtering is not successful. Have you tested tracepoint? @tpapagian

@Y-dc To be honest, most of the testing is done in kprobes. Can you please provide an example tracing policy that fails to work in tracepoints?

FYI, #774 will fix an issue in matchBinaries but this is not related only to tracepoints.

Using the following tracing policy: kprobes: - call: "fd_install" syscall: false args: - index: 0 type: int - index: 1 type: "file" selectors: - matchBinaries: - operator: "In" values: - "/usr/bin/cat" matchArgs: - index: 1 operator: "Equal" values: - "/etc/passwd" - matchBinaries: - operator: "In" values: - "/usr/bin/tail" matchArgs: - index: 1 operator: "Equal" values: - "/etc/shadow" we expect to get events if: [ binary == /usr/bin/cat AND arg1 == /etc/passwd ] OR [ binary == /usr/bin/tail AND arg1 == /etc/shadow ] Using the previous tracing policy and running: /usr/bin/cat /etc/passwd && /usr/bin/cat /etc/shadow && /usr/bin/tail /etc/passwd && /usr/bin/tail /etc/shadow We get events for all of these. The issue is that in #686 we use a single map for all matchBinaries selectors. That makes the previous tracing policy to behave as: [ (binary == /usr/bin/cat OR binary == /usr/bin/tail) AND arg1 == /etc/passwd ] OR [ (binary == /usr/bin/cat OR binary == /usr/bin/tail) AND arg1 == /etc/shadow ] This patch fixes that issues. We move from a BPF_MAP_TYPE_HASH to a BPF_MAP_TYPE_HASH_OF_MAPS. We index the outter map based on selector ID and the inner map is exactly the same as the one we used in #686. Based on the previous tracing policy, the user side will generate the names_map and sel_names_map. names_map is shared among all selectors that have matchBinaries. Each distinct binary name will appear in the names_map with a unique value per entry (> 0). The contents of the names_map for the previous tracing policy will be: names_map["/usr/bin/cat"] = 1 names_map["/usr/bin/tail"] = 2 Whenever we have an execve event, we check the execve binary name and if is that inside names_map, we set execve_map_value->binary to the value of that entry. If we cannot find that entry inside names_map, we set execve_map_value->binary equals to 0 which means that this binary name is nowhere in all matchBinaries. Based in that we also generate the sel_names_map. This is a BPF_MAP_TYPE_HASH_OF_MAPS map where we index in the outter map using the selector ID and we index in the inner map using the execve_map_value->binary. If that exists, then matchBinary will be matched. The contents of the sel_names_map based on the previous tracing policy will be: sel_names_map[0 /* sel_index */ ] = { hash_map[1 /* value of cat in names_map */] = 1 /* always 1 for now */ } sel_names_map[1 /* sel_index */ ] = { hash_map[2 /* value of tail in names_map */] = 1 /* always 1 for now */ } Finally, if sel_names_map[sel_idx] is NULL this means that the specific selector does not container a matchBinaries action. If sel_names_map[sel_idx].hash_map[idx] is NULL this means that we don't care about the specific binary. Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>

tpapagian force-pushed the pr/apapag/match_binaries branch 3 times, most recently from b00b0e8 to 8165a88 Compare February 7, 2023 13:56

tpapagian changed the title ~~matchBinaries test~~ matchBinaries fixes Feb 7, 2023

tpapagian force-pushed the pr/apapag/match_binaries branch 2 times, most recently from bc99e24 to e1c331a Compare February 8, 2023 11:43

tpapagian marked this pull request as ready for review February 8, 2023 12:33

tpapagian requested a review from a team as a code owner February 8, 2023 12:33

tpapagian requested a review from jrfastab February 8, 2023 12:33

kkourt requested changes Feb 13, 2023

View reviewed changes

pkg/selectors/selectors.go Outdated Show resolved Hide resolved

tpapagian force-pushed the pr/apapag/match_binaries branch from e1c331a to 6044af8 Compare February 13, 2023 19:48

mtardy mentioned this pull request Feb 20, 2023

Improve README and document tracing policies #673

Merged

tpapagian marked this pull request as draft February 20, 2023 10:28

tpapagian force-pushed the pr/apapag/match_binaries branch 3 times, most recently from 8c9857a to cae406c Compare February 20, 2023 16:24

tpapagian marked this pull request as ready for review February 21, 2023 09:23

olsajiri reviewed Feb 22, 2023

View reviewed changes

bpf/process/bpf_execve_event.c Show resolved Hide resolved

kkourt requested changes Feb 23, 2023

View reviewed changes

pkg/selectors/kernel.go Show resolved Hide resolved

bpf/process/bpf_execve_event.c Show resolved Hide resolved

bpf/process/types/basic.h Show resolved Hide resolved

pkg/selectors/selectors.go Show resolved Hide resolved

bpf/process/types/basic.h Outdated Show resolved Hide resolved

kkourt added the release-blocker This PR or issue is blocking the next release. label Feb 23, 2023

tpapagian force-pushed the pr/apapag/match_binaries branch from d260024 to 2f67be5 Compare February 23, 2023 10:03

tpapagian mentioned this pull request Feb 23, 2023

matchBinaries follow-ups #722

Closed

tpapagian force-pushed the pr/apapag/match_binaries branch from 2f67be5 to b67be11 Compare February 23, 2023 10:17

kkourt approved these changes Feb 23, 2023

View reviewed changes

tixxdz reviewed Feb 24, 2023

View reviewed changes

bpf/process/bpf_execve_event.c Outdated Show resolved Hide resolved

pkg/selectors/kernel.go Show resolved Hide resolved

pkg/selectors/selectors.go Show resolved Hide resolved

pkg/selectors/selectors.go Outdated Show resolved Hide resolved

bpf/lib/generic.h Outdated Show resolved Hide resolved

tpapagian added 8 commits February 24, 2023 10:09

Add NotIn operator for matchBinaries

84e71d2

For now we only supported In operator in matchBinaries. This patch adds support for the NotIn operator. Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>

Fix names_map update in generic tracepoints

ceec3cc

After loading a tracepoint program we should update names_map with new enties in a similar way to kprobes. Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>

Add tests for matchBinaries

b5d2ef4

Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>

Convert max binary size to a define

2fc6811

Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>

tpapagian force-pushed the pr/apapag/match_binaries branch from b67be11 to 2fc6811 Compare February 24, 2023 10:10

tpapagian changed the title ~~matchBinaries fixes~~ matchBinaries improvements Feb 24, 2023

tixxdz approved these changes Feb 24, 2023

View reviewed changes

tpapagian merged commit 640361d into main Feb 24, 2023

tpapagian deleted the pr/apapag/match_binaries branch February 24, 2023 10:42

tpapagian mentioned this pull request Mar 8, 2023

bugfix: matchBinaries in multiple selectors #774

Merged

tpapagian mentioned this pull request Mar 31, 2023

tetragon: fixup binary names selectors #521

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

matchBinaries improvements #686

matchBinaries improvements #686

tpapagian commented Feb 6, 2023 •

edited

mtardy commented Feb 20, 2023

tpapagian commented Feb 20, 2023

tpapagian commented Feb 20, 2023

tpapagian commented Feb 21, 2023

kkourt left a comment

tpapagian commented Feb 23, 2023

kkourt left a comment

tixxdz left a comment

tpapagian commented Feb 24, 2023

mtardy commented Feb 24, 2023

Y-dc commented Mar 20, 2023 •

edited

tpapagian commented Mar 20, 2023 •

edited

matchBinaries improvements #686

matchBinaries improvements #686

Conversation

tpapagian commented Feb 6, 2023 • edited

mtardy commented Feb 20, 2023

tpapagian commented Feb 20, 2023

tpapagian commented Feb 20, 2023

tpapagian commented Feb 21, 2023

kkourt left a comment

Choose a reason for hiding this comment

tpapagian commented Feb 23, 2023

kkourt left a comment

Choose a reason for hiding this comment

tixxdz left a comment

Choose a reason for hiding this comment

tpapagian commented Feb 24, 2023

mtardy commented Feb 24, 2023

Y-dc commented Mar 20, 2023 • edited

tpapagian commented Mar 20, 2023 • edited

tpapagian commented Feb 6, 2023 •

edited

Y-dc commented Mar 20, 2023 •

edited

tpapagian commented Mar 20, 2023 •

edited